New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrading GRPC version to 1.33, Netty to 4.1.50Final and ETCD client driver #2582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
but unfortunately Integration tests of the Etcd metadata driver are failing```
Errors:
Error: org.apache.bookkeeper.metadata.etcd.Etcd64bitIdGeneratorTest.testGenerateIdParallel
Error: Run 1: Etcd64bitIdGeneratorTest.setUp:57->EtcdTestBase.setUp:79->EtcdTestBase.newEtcdClient:61 » NoSuchMethod
Error: Run 2: Etcd64bitIdGeneratorTest.setUp:57->EtcdTestBase.setUp:79->EtcdTestBase.newEtcdClient:61 » NoSuchMethod
Error: Run 3: Etcd64bitIdGeneratorTest.setUp:57->EtcdTestBase.setUp:79->EtcdTestBase.newEtcdClient:61 » NoSuchMethod
[INFO]
pom.xml
Outdated
@@ -130,7 +130,8 @@ | |||
<freebuilder.version>1.14.9</freebuilder.version> | |||
<google.code.version>3.0.2</google.code.version> | |||
<google.errorprone.version>2.1.2</google.errorprone.version> | |||
<grpc.version>1.18.0</grpc.version> | |||
<grpc.version>1.35.0</grpc.version> | |||
<tomcat-annotations.version>1.35.0</tomcat-annotations.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that the tomcat-annotations.version
is not correct. What is the exact reason for introducing this dependency?
It looks like the dependency isn't maintained, updated last in 2017: https://search.maven.org/artifact/org.apache.tomcat/annotations-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is resolved.
The problem with the integration tests seems to be related to the etcd client. I am not sure what/whom this can affect but anyone running 2yo etcd should consider rethinking their life choices. :/ |
691ce7a
to
e2809b0
Compare
…de grpc a little (to 1.33) to get integration tests to work
So far: I left netty at its current version. jetcd dependencies use newer version but TLS tests timeout with it A few notes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work.
I will check tomorrow the hardest parts.
Aren't we updating LICENSE files?
@eolivelli The licenses weren't updated in a while. I.e. licenses still referred netty 4.1.32, the build is on 4.1.50, similarly others (rocksdb etc).
This is what's currently left for the server, after the last commit:
|
metadata-drivers/etcd/pom.xml
Outdated
<dependency> | ||
<groupId>org.testcontainers</groupId> | ||
<artifactId>testcontainers</artifactId> | ||
<scope>test</scope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is <scope>test</scope>
removed from testcontainers dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved by 2d86dc7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I placed one question about dependencies in metadata-drivers/etcd/pom.xml
. The <scope>test</scope>
was removed for testcontainers dependency. However it seems odd that the same file contains other test dependencies such as
<dependency>
<groupId>org.arquillian.cube</groupId>
<artifactId>arquillian-cube-docker</artifactId>
<version>${arquillian-cube.version}</version>
<exclusions>
<exclusion>
<groupId>com.github.docker-java</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.jboss.arquillian.junit</groupId>
<artifactId>arquillian-junit-standalone</artifactId>
<version>${arquillian-junit.version}</version>
<exclusions>
<exclusion>
<groupId>com.github.docker-java</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
Is this intentional?
<dependency> | ||
<groupId>org.arquillian.cube</groupId> | ||
<artifactId>arquillian-cube-docker</artifactId> | ||
<version>${arquillian-cube.version}</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have <scope>test</scope>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved by 2d86dc7
<dependency> | ||
<groupId>org.jboss.arquillian.junit</groupId> | ||
<artifactId>arquillian-junit-standalone</artifactId> | ||
<version>${arquillian-junit.version}</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have <scope>test</scope>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved by 2d86dc7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Descriptions of the changes in this PR:
upgraded GRPC
Will need to upgrade GRPC in pulsar as well
Upgrading:
Motivation
TableService stopped working under load.
jstack points to a problem with GRPC that is fixed in later versions
Changes
upgraded GRPC
Master Issue: #2581